Skip to content

core: fix security and concurrency issues found in a backend audit#2805

Open
asteroidzman wants to merge 1 commit into
AvengeMedia:masterfrom
asteroidzman:asteroidz-security-fixes
Open

core: fix security and concurrency issues found in a backend audit#2805
asteroidzman wants to merge 1 commit into
AvengeMedia:masterfrom
asteroidzman:asteroidz-security-fixes

Conversation

@asteroidzman

Copy link
Copy Markdown

While auditing the core backend for a downstream fork, I found a few security and concurrency issues that apply to upstream as well. Each is an independent, self-contained fix; happy to split into separate PRs if you'd prefer.

Security

  • privesc: pipe the sudo password via stdin (sudo -S) instead of embedding it in the command string, so it no longer appears in argv (readable by any local user via /proc/<pid>/cmdline or ps).
  • greeter: tokenize a session .desktop Exec= line into argv and execve directly instead of running it through /bin/sh -c, closing a command-injection path via user-writable ~/.local/share/wayland-sessions.
  • plugins: reject path-separator / .. in plugin id/name before joining into a filesystem path, closing an arbitrary-directory-delete in the uninstall/update fallback.

Concurrency / robustness

  • NetworkManager backend: guard shared state accessed from D-Bus signal handlers, fix a few signal races, and tighten ethernet/wifi state handling.
  • clipboard, cups, freedesktop, router/server: fix data races and lifecycle issues found alongside the above.

Notes

  • Touches core/ only — no UI/QML changes.
  • make build, go vet, and the keybinds tests pass on current master.

Security:
- privesc: pipe the sudo password via stdin (sudo -S) instead of
  embedding it in the command string, so it no longer appears in argv
  (readable by any local user via /proc/<pid>/cmdline or ps)
- greeter: tokenize a session .desktop Exec= line into argv and execve
  directly instead of running it through /bin/sh -c, closing a command-
  injection path via user-writable ~/.local/share/wayland-sessions
- plugins: reject path-separator/.. in plugin id/name before joining
  into a filesystem path, closing an arbitrary-directory-delete in the
  uninstall/update fallback
- keybinds/hyprland: always quote unrecognized bind actions/keys when
  writing generated Lua; only re-emit genuine round-tripped custom Lua
  verbatim (tracked via an explicit flag), closing a Lua-injection path
- desktop/mimeapps: reject newline/bracket in mime/desktop-id fields so
  they can't inject fake sections into the shared mimeapps.list

Robustness / concurrency:
- server: recover panics in the request-dispatch path so one bad
  handler can't crash the daemon and drop every client
- go-wayland: recover panics in the shared dispatch choke point so a
  malformed compositor event can't crash CLI tools / the daemon
- server: per-connection D-Bus client ID instead of a shared constant,
  fixing cross-client signal delivery and subscription teardown
- network: guard the NetworkManager device maps with a mutex (a
  concurrent map read/write here is an unrecoverable fatal error)
- cups: close the event channel on Stop() so Unsubscribe() of the last
  subscriber no longer deadlocks; allocate the fresh channel in Start()
- freedesktop: reuse the shared session conn for the settings watcher
  and tear it down in Close(), fixing a per-Manager conn+goroutine leak
- clipboard: mutex-guard lazy dbusConn creation
- geolocation: use WithMatchMember for the GeoClue2 LocationUpdated
  signal (was WithMatchSender with an interface.member string, so the
  match never fired and live location updates never arrived)
- screenshot: set failed=true on buffer/pool creation errors so the
  dispatch loop doesn't wait forever for a ready/failed that never comes

@bbedward bbedward left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of the changes are good, but I'm skeptical about the dbus ID changes breaking clients. That needs verification, as well as ensuring the greeter launcher changes are adhering to XDG specs, and generally flattening the verbosity of comments

// of the same clientID formula for the streaming "subscribe"
// endpoint, which must stay in sync with this one).
clientID := fmt.Sprintf("meta-client-%p", conn)
serverDbus.HandleRequest(conn, req, dbusManager, clientID)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect this probably breaks client signal delivery because ClientID lookup wont match in the QMl-side. Same problem in server.go, you can test plugins like DankKDEConnect to see if they still work afterwards

// bug in any one handler (bad type assertion, nil deref, out-of-range
// index, etc. -- reachable from client-controlled req.Method/req.Params)
// can't crash the whole daemon and disconnect every other client. It
// reports back to the offending client as a normal error response instead.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to cut down all of these comments, its way too verbose for the code its covering which is largely self-documenting anyway

// basic single/double quoting and backslash escapes, but never invokes a
// shell. Session .desktop files can live in a user-writable directory
// (~/.local/share/wayland-sessions), so shell metacharacters in Exec=
// must never reach an actual shell -- they're just literal argv text here.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a security fix since the user is already authenticated at this point, but maybe a spec correctness change

hasCur = false
}
i++
case c == '\'':

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe XDG spec dictates single quotes shouldnt be treated as a quote, but a reserved character


argv := make([]string, 0, len(tokens))
for _, tok := range tokens {
if strings.HasPrefix(tok, "%") {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would maybe reference quickshell code DeskopEntries for all of this stuff, to ensure its following the XDG specification

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants